[pkg] Resolve directory symlinks in fetched targets by ElectreAAS · Pull Request #13792 · ocaml/dune

ElectreAAS · 2026-03-13T16:18:11Z

Housekeeping

This PR fixes the tests in #13393
#9873 will not be fixed, but still a significant step towards fixing #13678.

What this PR does

After fetching package sources, add a pass resolving directory symlinks. As they're not problematic at this stage, file symlinks are left as is. Broken symlinks are removed silently to preserve existing behaviour.

Note: both portable_hardlink and portable_symlink work backwards from what I initially understood, see #13791

Done with the help of @Alizter, so thanks :)

Alizter

Here is an example that I think causes a loop in the current algorithm:

  /source/
    dir_a/
      link_to_b -> ../dir_b
    dir_b/
      link_to_a -> ../dir_a

Following this along we get an infinitely descending directory.

I think you will need to keep a set of resolved targets and check against it if you want to remember where you have been as to not repeat the resolution.

Another issue that I see is the multiple passes that are currently done, but we can improve this later once we have something that works correctly.

We also might need to add some validation that we don't escape the source directory, I don't think its a good idea to allow symlinks to / for example.

ElectreAAS · 2026-03-26T22:44:54Z

I just pushed work in progress to adress the latest comment about cycles.
A basic cycle detection mechanism is in place, and works in simple scenarios (basic-cycle.t), but fails in more complex ones (link-to-parent.t) and crashes the sandboxing mechanisms of cram tests. Fun stuff

ElectreAAS · 2026-04-09T16:19:58Z

+This fails correctly
+  $ build_pkg bar 2>&1 | sanitize_pkg_digest bar.0.0.1 | tail -3
+  Error: Unable to resolve symlink
+  _build/_private/default/.pkg/bar.0.0.1-DIGEST_HASH/source/dir_b/link_to_a/link_to_b,


CI isn't happy because it finds dir_a/link_to_b/link_to_a before the one I wrote in the test

ElectreAAS · 2026-04-09T16:21:26Z

Aside from the CI failures due to non-deterministic errors, the main code is ready for review.
Special attention should be given to the different Path.something.t, I had to hit them with a hammer until they looked nice, but I may very well have misunderstood the different invariants at play

rgrinberg · 2026-04-09T18:38:35Z

I haven't looked closely, but I think your changes might have made that test non-deterministic.

Alizter

Here are some problems I've observed:

There is a difference between local and tarball sources when it comes to directory symlinks. The tarball sources correctly resolve the contents wheras local sources silently discard them. I think rather than silently discarding them we should either raise a user error if this is something we wish not to support or support it. I would probably consider not supporting it.
Broken symlinks appear to be silently ignored. For example something stupid like a symlink to itself. We should probably error to the user in this case saying that we don't accept such symlinks.

Alizter · 2026-04-14T14:08:50Z

+             (* [raw_resolved] is a relative build path but it might contain indirections,
+                something like _build/foo/../bar
+                or _build/../outside *)
+             let canon_resolved = Path.of_string raw_resolved in


This only canonicalises relative paths and not absolute ones. I think Path.External.canonicalize_abs was the other way you did it.

For some reason, raw_resolved is always a relative path, which is correctly canonicalized by Path.of_string
I ended up removing Path.External.canonicalize_abs anyway

Alizter · 2026-04-14T14:42:36Z

Here are some of the issues we currently have:

Test symlink chains: A -> B -> C where intermediate links are also symlinks.

  $ mkdir -p _src/real_dir
  $ echo "content" > _src/real_dir/file.txt
  $ ln -s real_dir _src/link_a
  $ ln -s link_a _src/link_b
  $ ln -s link_b _src/link_c

  $ make_lockdir

Case 1: local source with symlink chain

For local sources, directory symlinks are not copied at all (they are skipped
during the source copy). Only the real directory is present.

  $ make_lockpkg foo <<EOF
  > (version 0.0.1)
  > (source
  >  (fetch
  >   (url file://$PWD/_src)))
  > (build (run cat real_dir/file.txt))
  > EOF

  $ build_pkg foo
  content

Only the real directory is copied - symlink chains are lost entirely:
  $ ls _build/_private/default/.pkg/foo.*/source | sort
  link_a
  link_b
  link_c
  real_dir

Case 2: tarball source with symlink chain

For tarballs, symlinks are preserved during extraction and then resolved.

  $ tar czf _src.tar.gz _src

  $ make_lockpkg bar <<EOF
  > (version 0.0.1)
  > (source
  >  (fetch
  >   (url file://$PWD/_src.tar.gz)))
  > (build (run cat real_dir/file.txt))
  > EOF

  $ build_pkg bar
  content

  $ ls _build/_private/default/.pkg/bar.*/source | sort
  link_a
  link_b
  link_c
  real_dir

BUG: Different behavior between local source and tarball. With local sources,
the symlink chain is completely lost (link_a, link_b, link_c are missing).
With tarballs, all links are resolved to real directories. This inconsistency
means a package behaves differently depending on how it is fetched.

ElectreAAS · 2026-04-20T09:57:57Z

Update: we've looked further into this and it seems the desired behaviour in Pkg_rules.source_files is rather complicated, almost equating actually supporting symlinks in the engine.
Since the point of this PR was to allow dune pkg to build fetched packages that may contain symlinks, and that we'd added the resolving pass in source_files only for symmetry, we came to the conclusion that it was unnecessary, and have removed it.

I've pushed a new version containing that deletion, along with a few WIP comments, I'm looking into them

ElectreAAS · 2026-04-22T20:10:00Z

I've extracted a few unnecessary changes to #14291 to make review as simple as possible.
I think I've adressed all concerns raised previously, this is as ready as it will be

Just a little change extracted from #13792, nothing changing dune behaviour. Figured I'd use Fpath.traverse where it's appropriate

Just a trivial simplification in `Pkg_rules.source_files`, nothing changing dune behaviour. Instead of `map`ping the `relative` part later, do it once in the partition. This isn't particularly useful on its own, but in case of changes inside the partition function, it makes debugging a lot easier Extracted from #13792

Alizter · 2026-04-28T17:43:24Z

+            seen, if symlinks_in_children then Some Unix.S_DIR else None)
+       | { Unix.st_kind; _ } ->
+         (* We do not care about symlinks pointing to anything but directories. *)
+         seen, Some st_kind)


You say we don't care, but won't returning Some st_kind mean further processing by Fpath.traverse? Wouldn't it be better to actually skip it with None?

Alizter · 2026-04-28T17:54:59Z

+         seen, Some st_kind)
+  in
+  let _symlinks_seen : String.Set.t =
+    Fpath.traverse


Question about Fpath.traverse: Is it possible for this function to be "tricked" into reading more than it bargained for? i.e. can you create more things for it to read as it is reading?

My follow up question is what's the difference between the two cycle detection mechanisms:

seen

is_descendent

Question about Fpath.traverse: Is it possible for this function to be "tricked" into reading more than it bargained for? i.e. can you create more things for it to read as it is reading?

Yes it's possible. If on_symlink returns a Some kind, traverse's handle_kind will be called on that kind. You could even create an infinite loop by making on_symlink return a directory that contains a symlink, that will then be turned into yet another directory...

What I'm doing is obviously not that pathological case, but since I am turning directory symlinks into regular directories, I do create more stuff for the traverse to read. This is intended

My follow up question is what's the difference between the two cycle detection mechanisms:
* seen
* is_descendent

I just checked and it turns out that seen is useless (at least in the tests we have so far). It was necessary at some point when is_descendant didn't work as I'd thought, but not anymore.
I'll try to stress-test it a little more, but maybe it can be removed

Yes, you can replace seen with () and simplify the rest a little bit more :-)

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

… source) Sort entries in fetch to guarantee determinism Remove the symlink resolution happening in pkg_rules as it's too complicated. The main change happening in fetch is still there. Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

The big comment explaining everything was moved to fetch. Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

…d function Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

Remove 'seen' as cycle detection, is_descendant is enough Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

ElectreAAS · 2026-04-29T19:15:39Z

I tested this with ocamlbuild and fstar, and it builds fine!

Alizter · 2026-04-29T20:02:01Z

Those OxCaml failures are real and are due to a new version of ppx_expect. It seems let%test_module is now replaced by module%test. I will fix this in another PR.

Signed-off-by: Ali Caglayan <alizter@gmail.com>

Alizter · 2026-04-29T20:27:40Z

Annoyingly OxCaml is using ppx_expect v18, but v18 hasn't been released so we have a situation where the OxCaml version of the build is using a newer library that cannot overlap with the old version we have available. Luckily we can detect that and pass some compatibility flags to make it work.

ElectreAAS commented Mar 13, 2026

View reviewed changes

Comment thread test/blackbox-tests/test-cases/pkg/source-with-directory-symlink.t

ElectreAAS force-pushed the push-xmyqtonwkonu branch from df327f2 to 91d83ae Compare March 16, 2026 17:22

ElectreAAS requested review from art-w and rgrinberg March 16, 2026 17:25

shonfeder requested a review from Alizter March 19, 2026 13:08

Alizter mentioned this pull request Mar 20, 2026

Relocatable compiler in dune #13229

Open

15 tasks

Alizter requested changes Mar 25, 2026

View reviewed changes

ElectreAAS marked this pull request as draft March 26, 2026 22:43

Alizter self-requested a review March 27, 2026 07:56

Alizter reviewed Mar 27, 2026

View reviewed changes

Comment thread src/dune_pkg/fetch.ml Outdated

Alizter reviewed Mar 27, 2026

View reviewed changes

Comment thread src/dune_pkg/fetch.ml Outdated

ElectreAAS force-pushed the push-xmyqtonwkonu branch 3 times, most recently from fe80747 to 5134ba5 Compare April 8, 2026 14:13

ElectreAAS mentioned this pull request Apr 9, 2026

Add comment explaining that Fpath.follow_symlink is recursive, delete follow_symlinks #14091

Merged

ElectreAAS force-pushed the push-xmyqtonwkonu branch from 5134ba5 to b869da0 Compare April 9, 2026 13:03

ElectreAAS commented Apr 9, 2026

View reviewed changes

ElectreAAS marked this pull request as ready for review April 9, 2026 16:20

ElectreAAS force-pushed the push-xmyqtonwkonu branch from c43ca48 to 6a8ab11 Compare April 10, 2026 10:39

Alizter requested changes Apr 14, 2026

View reviewed changes

Alizter reviewed Apr 14, 2026

View reviewed changes

Comment thread otherlibs/stdune/src/path.ml Outdated

Alizter reviewed Apr 14, 2026

View reviewed changes

Comment thread src/dune_rules/pkg_rules.ml Outdated

ElectreAAS force-pushed the push-xmyqtonwkonu branch 2 times, most recently from bc64415 to 71bc486 Compare April 20, 2026 09:50

ElectreAAS force-pushed the push-xmyqtonwkonu branch from 71bc486 to 7a3f5e6 Compare April 20, 2026 10:26

ElectreAAS force-pushed the push-xmyqtonwkonu branch 5 times, most recently from bdb9bfd to 2ba65a9 Compare April 22, 2026 20:04

ElectreAAS mentioned this pull request Apr 22, 2026

Use Fpath.traverse in pkg_rules #14291

Merged

ElectreAAS mentioned this pull request Apr 23, 2026

Simplify Pkg_rules.source_files a bit #14301

Merged

Alizter added a commit that referenced this pull request Apr 23, 2026

Use Fpath.traverse in pkg_rules (#14291)

d7f46a7

Just a little change extracted from #13792, nothing changing dune behaviour. Figured I'd use Fpath.traverse where it's appropriate

ElectreAAS force-pushed the push-xmyqtonwkonu branch from 2ba65a9 to 46866ea Compare April 28, 2026 16:59

Alizter reviewed Apr 28, 2026

View reviewed changes

Comment thread src/dune_pkg/fetch.ml Outdated

ElectreAAS added 7 commits April 29, 2026 13:11

Allow Path.is_descendant to work on external paths.

7a8c64e

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

Remove superseded symlink exclusion logic from fetch_rules.

24b3864

The big comment explaining everything was moved to fetch. Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

Fix 'seen' accumulator not being used inside the fold

cc17f6f

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

Cleanup test cases

f3acbc3

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

Enshrine in test case that the git pin workflow is also fixed

7c8dec1

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

Added inline tests to resolve_directory_symlinks as it's a complicate…

3c09dea

…d function Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

Alizter reviewed Apr 29, 2026

View reviewed changes

Comment thread src/dune_pkg/fetch.ml Outdated

Use Fpath.traverse in place of doing the traversal manually

cafeb8b

Remove 'seen' as cycle detection, is_descendant is enough Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

ElectreAAS force-pushed the push-xmyqtonwkonu branch from 46866ea to cafeb8b Compare April 29, 2026 15:56

Added comments

b6df243

Signed-off-by: Ambre Austen Suhamy <ambre@tarides.com>

fix: add -inline-test-allow-let-test-module for OxCaml compat

ee52e3b

Signed-off-by: Ali Caglayan <alizter@gmail.com>

ElectreAAS mentioned this pull request Apr 30, 2026

Allow Path.is_descendant to work on external paths. #14382

Open

Conversation

ElectreAAS commented Mar 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Housekeeping

What this PR does

Uh oh!

Uh oh!

Alizter left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElectreAAS commented Mar 26, 2026

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ElectreAAS commented Apr 9, 2026

Uh oh!

rgrinberg commented Apr 9, 2026

Uh oh!

Alizter left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Alizter commented Apr 14, 2026

Uh oh!

ElectreAAS commented Apr 20, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ElectreAAS commented Apr 22, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ElectreAAS commented Apr 29, 2026

Uh oh!

Alizter commented Apr 29, 2026

Uh oh!

Alizter commented Apr 29, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ElectreAAS commented Mar 13, 2026 •

edited

Loading

Alizter left a comment •

edited

Loading

ElectreAAS commented Apr 20, 2026 •

edited

Loading